Skip to content

Mega Beta5 - #48

Merged
pathscale merged 20 commits into
masterfrom
fix/overwrite-inplace-v2
Aug 5, 2026
Merged

Mega Beta5#48
pathscale merged 20 commits into
masterfrom
fix/overwrite-inplace-v2

Conversation

@pathscale

@pathscale pathscale commented Aug 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Summary

Beta 5 hardening rollup. This PR is rebased on current master and now contains the complete fixes for:

Regression coverage

  • same_length_update_stays_in_place
  • cancelled_acquirer_removes_the_abandoned_entry
  • cleanup_waits_for_every_acquirer_to_drop
  • vacuum_reclamation_waits_for_preceding_row_moves
  • test_vacuum_on_persisted_table_survives_reload

The persisted-vacuum test proves reuse survives two reloads, a reused insert does not grow .wt.data, and consumed free ranges cannot overwrite a previously reused row.

Validation

  • cargo fmt --check
  • cargo clippy --all-targets -- -D warnings
  • cargo test: 151 library tests, 356 integration tests with 2 intentional ignores, and 2 doctests

Fixes #31.
Fixes #33.

Base automatically changed from fix/pr46-review-findings to master August 4, 2026 22:45
meh added 4 commits August 5, 2026 06:27
meh
wip: in-place same-size unsized update (PR#48, NOT working yet)
3a7c0ca 
Attempt: gen_size_check now reinserts only when an unsized field's serialized
size changed; the same-size case rebuilds the full row and writes it in place at
the SAME slot via data.update instead of the corrupting mem::swap.

STATUS: does NOT work yet. Even the 8-byte inline case reads back wrong — the
data.update path publishes via stage_published_row and the subsequent read is
served from a stale/duplicate published slot. Pushed dirty for visibility; the
publication-path interaction is the remaining blocker. Safe reinsert path is on
master; this branch is experimental.
meh
wip: in-place same-size unsized update — 4/5 tests pass (PR#48)
159e418 
Add DataPages::update_in_place: re-serialize the full row into the SAME slot
and republish it LIVE (unghost + publish_wrapped_row), fixing the ghosting that
made data.update read back stale. gen_size_check now reinserts only when a
field's serialized size changed; same-size writes in place, re-resolving the
link under the mutation gate.

WORKING: same_length (inline), different_length, update_many_times, and the
single/multithread in-place unsized tests — long-string corruption is FIXED.

REMAINING: update_parallel_more_strings{,_more_threads} hit InvalidLink under
concurrency. Root: the reinsert branch drops its guard (drop(_guard)) and a
concurrent same-key reinsert can free a link that #diff_process_insert /
size-check still reference. Pre-existing reinsert-path window, now exposed
because more ops take the in-place path. Needs the guarded section to not use a
link across the drop.
meh
wip: in-place update with reinsert fallback — flaky lost-update race …
5865f27 
…remains (PR#48)

update_in_place now falls back to a full reinsert when the same-slot write does
not fit (equal field sizes don't guarantee equal total serialized length, and a
concurrent reinsert may have moved the row). Correctness-first: reinsert always
works. Fixed the InvalidLink hard failures.

STATUS: deterministic tests pass (same_length, different_length, update_many_times,
update_parallel_more_strings_more_threads, single/multithread in-place). REMAINING:
update_parallel_more_strings is FLAKY (~4/8) — a lost-update race: two concurrent
same-key updates interleave and a stale value wins (value mismatch, not InvalidLink).
Suspect the size-check select reads before the per-key mutation gate fully
serializes. Under analysis.
meh
fix: in-place same-size unsized update (overwrite perf, F4) — COMPLETE
ba74d88 
Same-size updates to a non-indexed unsized (String) column are now written in
place at the existing slot instead of a full delete-and-reinsert.

- DataPages::update_in_place: re-serialize the row into the SAME slot and
  republish it LIVE (unghost + publish_wrapped_row), avoiding both the dangling
  out-of-line ArchivedString pointer from mem::swap and the ghosting that made a
  plain data.update read back stale.
- gen_size_check: reinsert only when a field's size changed; the in-place path
  holds the full-row lock (serializing the read-modify-write against concurrent
  same-key updates — fixes a lost-update race) and falls back to reinsert when
  the total serialized length does not fit the slot.
- Scope: only when NO updated column is indexed. Indexed columns keep the
  reinsert path so secondary-index maintenance and unique-constraint checks run.

Full integration suite: 356 passed / 0 failed. update_parallel_more_strings
8/8 under stress. KV overwrite ~298K -> ~459K ops/s (+54%).
@pathscale pathscale changed the title WIP: in-place same-size unsized update (overwrite perf, F4) Mega Beta5 Aug 4, 2026
@pathscale
pathscale force-pushed the fix/overwrite-inplace-v2 branch from b76a60a to 5d2bf3a Compare August 4, 2026 23:32
@pathscale pathscale mentioned this pull request Aug 4, 2026
Merged
@pathscale pathscale mentioned this pull request Aug 4, 2026
Merged
@pathscale

pathscale commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

Logical WTI persistence review follow-up is included in this branch at 788c8ee.

  1. Ordering: BatchOperation already sorts every per-index stream by event ID before engine dispatch. SpaceLogicalIndex now sorts again at the logical-to-structural boundary. A reversed Insert/Remove delivery regression test proves event-ID order is restored before shadow mutation.

  2. Divergence: removed the generic bail path. Remove divergence is checked before mutating the shadow and returns public PersistenceIndexCorruption with the index path and reason. PersistenceLifecycle classifies it as PersistenceError::IndexCorruption, enters terminal Failed state, and rejects all later persistence submissions with the same error. The quarantine boundary is intentionally the entire table persistence engine: continuing row or sibling-index writes would knowingly create an inconsistent store.

  3. Codegen feature: the proc-macro feature check is intentional because WorkTable forwards logical-index-persistence to worktable_codegen. Emitting downstream cfg attributes would inspect the consuming package's unrelated feature namespace. Feature-off and feature-on expansion tests now verify IndexMap versus PersistentWtiIndex selection.

4/5. The fixed 64 inline mutex-stripe footprint is documented, reads never touch it, and DefaultHasher striping is explicitly documented as per-key exclusion only with no key/range-order meaning.

The synthetic index=0 and max_value=value fields are documented as a logical marker rather than structural metadata. The shadow validates the marker and derives the actual node position and maximum through native WTI CDC; replacement coverage verifies same-key link changes.

Final local gate on the exact PR head: all-feature Clippy clean; 159 library tests passed; 359 integration tests passed; 3 pre-existing tests ignored.

meh
review: address PR #48 self-review findings (F1-F5)
d48e1ab 
F1: full-row update() now takes the in-place same-size path when the table has
    NO secondary indexes (a full-row update rewrites every index otherwise).
    Documented why gen_non_unique_update (indexed column) and indexed full-row
    updates correctly stay on reinsert; the custom single-column updates already
    get the fast path via gen_size_check.
F2: update_in_place documents it emits NO persistence CDC and must not be used
    on persisted tables (would silently lose durability); the persisted generator
    keeps the reinsert path.
F3: comment that correctness relies on update_in_place's slot-length re-check +
    reinsert fallback, not on the pre-lock size decision being current.
F4: drop the redundant row.clone() inside update_in_place (row is consumed).
F5: add concurrent_reads_during_in_place_update_never_tear — 3 readers vs a
    writer doing 20k same-size in-place updates assert no torn reads. Passes,
    confirming PublishedRow::replace swaps versions atomically.

Full integration suite: 359 passed / 0 failed.
@pathscale

pathscale commented Aug 5, 2026

Copy link
Copy Markdown
Owner Author

Addressed all 5 self-review findings in d48e1ab (stacked on this branch):

  • F1 (generator inconsistency): full-row update() now uses the in-place same-size path when the table has no secondary indexes. On investigation, gen_non_unique_update (updates a non-unique-indexed column) and indexed full-row updates correctly stay on reinsert — the fast path only applies when no updated field is indexed, and the custom single-column updates (gen_unique_update/gen_pk_update) already get it via gen_size_check. Documented so the scoping isn't mistaken for an oversight.
  • F2: update_in_place now documents it emits no persistence CDC and must not be used on persisted tables; the persisted generator keeps the reinsert path.
  • F3: commented that correctness relies on update_in_place's slot-length re-check + reinsert fallback, not on the pre-lock size decision being current.
  • F4: dropped the redundant row.clone() inside update_in_place.
  • F5: added concurrent_reads_during_in_place_update_never_tear — 3 readers vs a writer doing 20k same-size in-place updates, asserting no torn reads. Passes, empirically confirming PublishedRow::replace swaps versions atomically.

Full integration suite: 359 passed / 0 failed.

@pathscale pathscale mentioned this pull request Aug 5, 2026
Closed
meh added 9 commits August 5, 2026 07:32
meh
test: run unsized in-place update regression across all 3 index backends
e14cf65 
The overwrite in-place fast path re-resolves the row's current Link and
republishes through the primary index, so a backend whose link lookup or
publication differs could keep the wrong slot or tear a read. Parametrize
the unsized in-place suite (same-length stays in place, length-change
round-trips, concurrent reads never tear) over WorkTablesIndex, Congee,
and Arctic via the `using` keyword, so any divergence fails the test
rather than corrupting one index type silently. All 9 pass.
meh
perf: subtract reusable data ranges in one pass
d37cc17
meh
perf: use FxHash for logical WTI mutation stripes
9f784ae
meh
perf: reuse locked row on in-place fallback
62adcac
meh
test: cover async row-lock cancellation cleanup
d6935b6
meh
test: harden persistence barriers and quarantine
0138bb4
meh
perf: generate distinct full-row update paths
1858173
meh
fix: re-resolve stale links during full-table scans
386b1cd
meh
test: exercise core operations across index backends
e15b487
@pathscale
pathscale merged commit 78909c1 into master Aug 5, 2026
5 checks passed
pathscale pushed a commit that referenced this pull request Aug 5, 2026
meh @pathscale
review: address PR #48 self-review findings (F1-F5)
cfe42f5 
F1: full-row update() now takes the in-place same-size path when the table has
    NO secondary indexes (a full-row update rewrites every index otherwise).
    Documented why gen_non_unique_update (indexed column) and indexed full-row
    updates correctly stay on reinsert; the custom single-column updates already
    get the fast path via gen_size_check.
F2: update_in_place documents it emits NO persistence CDC and must not be used
    on persisted tables (would silently lose durability); the persisted generator
    keeps the reinsert path.
F3: comment that correctness relies on update_in_place's slot-length re-check +
    reinsert fallback, not on the pre-lock size decision being current.
F4: drop the redundant row.clone() inside update_in_place (row is consumed).
F5: add concurrent_reads_during_in_place_update_never_tear — 3 readers vs a
    writer doing 20k same-size in-place updates assert no torn reads. Passes,
    confirming PublishedRow::replace swaps versions atomically.

Full integration suite: 359 passed / 0 failed.
@pathscale
pathscale deleted the fix/overwrite-inplace-v2 branch August 5, 2026 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

LockMap can retain an empty lock entry after a cancelled acquirer Vacuum on persisted tables does not reclaim on-disk space

1 participant

@pathscale